fix(datasets): decouple datasets feature from std_rand - #499

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.59%. Comparing base (9eaae9e) to head (ab2f6f8).
⚠️ Report is 204 commits behind head on main.

@@             Coverage Diff             @@
##             main     #499       +/-   ##
===========================================
+ Coverage   43.97%   85.59%   +41.61%     
===========================================
  Files          85       96       +11     
  Lines        7281     9379     +2098     
===========================================
+ Hits         3202     8028     +4826     
+ Misses       4079     1351     -2728     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Thanks for this, @ShryukGrandhi. The change is small and the approach is sound: datasets = ["serde"], generators routed through rand_custom::get_rng_impl(None), and TemporaryTextFile no longer needing an OS RNG. I read the diff and src/rand_custom.rs; I have not built or run it locally. Notes below, most important first.

1. Silent behaviour change for datasets-only users (main concern)
With datasets and no std_rand, get_rng_impl(None) returns SmallRng::seed_from_u64(0). So make_blobs, make_circles and make_moons now return identical data on every call. Anyone who relied on datasets pulling in std_rand gets constant data instead of a compile error, which is easy to miss (for example, a benchmark that now always sees the same blobs). The CHANGELOG entry covers this, but I'd go further:

• Add seeded variants or a seed parameter, e.g. make_blobs_with_seed(..., seed: Option<u64>), so callers can choose reproducibility without std_rand. The generators are the only place where get_rng_impl(None) is hard-wired.
• Document the determinism on the public functions' rustdoc, not just the module doc.
• Since this is a 0.x crate and the change is observable, label it Changed (breaking) in the CHANGELOG and make sure it lands in a minor bump.

2. js seeding is weak (pre-existing, but now more reachable)
In get_rng_impl, the js branch fills 64 bytes from getrandom but seeds with buf[0] as u64, so there are only 256 possible seeds. With this PR, datasets + js users hit that path through the generators. It would be better to build the seed from 8 of the bytes (u64::from_le_bytes(buf[..8].try_into().unwrap())). That is outside this PR's scope, so a follow-up issue is fine.

3. Tests

• test_generators_without_std_rand_are_reproducible is useful, but it only runs when both std_rand and js are off. Please make sure CI actually exercises that configuration, since --all-features will skip it.
• Consider a counterpart under #[cfg(feature = "std_rand")] asserting two make_blobs calls differ (with enough samples that a collision is practically impossible). That guards the 'stays random as before' claim in the description.
• Add CI jobs for cargo build --no-default-features --features datasets and cargo test --no-default-features --features datasets. Otherwise the decoupling can regress unnoticed, which is how no-std Step 3: decouple datasets feature from std_rand in Cargo.toml #416 arose.

4. TemporaryTextFile
PID plus an atomic counter is a good fix. Two small points: the file is still created in the current working directory (pre-existing), so std::env::temp_dir() would avoid littering the repo if a test panics before Drop. A leftover file from a crashed run with a recycled PID would be overwritten rather than collide, which is acceptable here.

5. Minor

• The Cargo.toml comment says 'Enables serde because datasets ship serialized bundles'. Please also state that std_rand must be added for non-deterministic generators, so the feature table is self-explanatory.
• Please confirm that rand's default features are not needed by anything else under datasets (e.g. rand::distr::Uniform and Distribution are available with default-features = false), which the --no-default-features build in your test plan already suggests.

Summary: the approach is good and I'd merge it after point 1 (docs or seed option plus a breaking-change note) and the CI coverage in point 3. Points 2 and 4 can be follow-ups.

cc @Mec-iS

Re-reviewed after the latest changes (d8d0837, 23ceda7 and the merge of main). I read the diff and the check runs; I have not built or run it locally. Thanks @ShryukGrandhi, this addresses nearly everything from my earlier review:

• Seeded variants make_blobs_with_seed, make_circles_with_seed and make_moons_with_seed (seed: Option<u64>), with the original functions delegating via None, so the public API is unchanged.
• Determinism is documented on each public generator, and the CHANGELOG entry is now Changed (breaking) with a minor-bump note.
• New std_rand randomness test and a seeded reproducibility test.
• CI now runs cargo test --no-default-features --features datasets.
• TemporaryTextFile uses std::env::temp_dir().
• The Cargo.toml comment now says to add std_rand for non-deterministic output.

All 13 checks pass on the current head (lint, MSRV 1.85, check_features including --features datasets, Linux/macOS/Windows, wasm32, i686, coverage).

Remaining points, none blocking:

1. js seeding (follow-up issue): get_rng_impl seeds from buf[0] as u64, which gives only 256 possible seeds. datasets with js now reaches it through the generators. Building the seed from 8 bytes (u64::from_le_bytes) would fix it. It is outside this PR, so a separate issue is fine.
2. Docs: make_circles_with_seed (and make_circles) panic when factor is outside 0..1. A # Panics section in the rustdoc would document that.
3. Test gap (minor): Nothing checks that *_with_seed(..., None) matches the plain function under the fixed-seed configuration. It is a one-line assertion if you want it.
4. CHANGELOG layout: Please check that the new Changed (breaking) and Added headings under [Unreleased] match the layout used elsewhere in the file.

Approving from my side once the maintainers are happy with point 4. cc @Mec-iS